fix: parallelize ensuring active pools on startup #1525
Conversation
Signed-off-by: Enrique Lacal <[email protected]>
| // by one of the goroutines. | ||
| if err := g.Wait(); err != nil { | ||
| // The above handleMessage will retry if errors occur | ||
| return err |
There was a problem hiding this comment.
This is a change in behavior - the previous ft.EnsureTokenPoolActive(ctx, currentPool) code did not crash the namespace start in the case it failed to activate a token pool.
It might be a positive change in behavior, but it would need some analysis/thinking (probably from @awrichar?) as to why the previous code intentionally did not fail on this condition before.
There was a problem hiding this comment.
Yeah I thought about that, it felt weird to me that it didn't fail on startup... Would love to hear @awrichar thoughts on this one
| @@ -716,16 +717,31 @@ func (ft *FFTokens) handleNamespaceStarted(ctx context.Context, data fftypes.JSO | |||
| // Make sure any pools that are marked as active in our DB are indeed active | |||
| namespace := data.GetString("namespace") | |||
| log.L(ctx).Debugf("Token connector '%s' started namespace '%s'. Ensuring all token pools active.", ft.Name(), namespace) | |||
|
|
|||
| g, ctx := errgroup.WithContext(ctx) | |||
There was a problem hiding this comment.
I feel like using `SetLimit() on the concurrency would be better than a completely unbounded parallelism against the token connector:
https://pkg.go.dev/golang.org/x/sync/errgroup#Group.SetLimit
There was a problem hiding this comment.
Good idea - will set that to the number of pools to activate
When a large number of token pools are active and namespace is restarted, it take a long time to activate one by one each token pool. This PR spins a new go routine for each API call.